fix: copy command sidecar directories during install#172
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCopies full module content trees into assistant-specific modules//, injects boundary module-dir preambles into generated skills/agents/commands when provided, synchronizes co-named command sidecar directories on install/uninstall, adds per-target get_module_path helpers, and updates docs and tests. ChangesModule install and multi-file command support
Sequence Diagram sequenceDiagram
participant CLI as Installer (install_to_assistant)
participant InstallerHelper as _install_module_tree
participant Target as AssistantTarget.generate_*
participant Helper as _inject_preamble
participant FS as Filesystem
CLI->>InstallerHelper: copy module tree -> modules/<name>/
InstallerHelper->>FS: create modules/<name>/ with nested files
CLI->>Target: call generate_skill/generate_command/generate_agent(module_dir=modules/<name>/)
Target->>Helper: pass content + module_dir
Helper->>Target: return injected content
Target->>FS: write generated files and copy/remove sidecar directories
CLI->>Target: on uninstall -> _uninstall_module_tree -> remove modules/<name> and installed artifacts
Estimated code review effort: Possibly related PRs:
Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d39bfa5d80
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_targets.py (1)
217-270: ⚡ Quick winAdd Cursor sidecar and OpenCode sidecar-removal coverage to complete the contract.
These tests validate the shared behavior well, but there’s still no explicit Cursor sidecar test and no OpenCode-side removal assertion (important because
OpenCodeTarget.remove_commandoverrides then delegates tosuper()).Also applies to: 961-975
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_targets.py` around lines 217 - 270, Add two tests mirroring the existing ClaudeCodeTarget sidecar behavior: one that ensures the Cursor sidecar directory (create a co-named directory next to the command source with nested files) is copied when calling the Cursor target's generate_command (reference CursorTarget or the Cursor-specific target class), and another that ensures OpenCodeTarget.remove_command actually removes the sidecar directory when uninstalling (assert that after calling OpenCodeTarget.remove_command the co-named directory under the destination no longer exists). Implement these tests analogous to test_generate_command_copies_sidecar_directory and test_generate_command_overwrites_existing_sidecar so they create source sidecars, call the appropriate target methods, then assert presence/absence of files; reference the generate_command method and OpenCodeTarget.remove_command to locate the relevant behavior to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lola/targets/base.py`:
- Around line 783-788: The current logic may call shutil.rmtree(sidecar_dest)
even when sidecar_dest is a file or symlink, causing an exception before
shutil.copytree; update the branch around sidecar_dest to check its type: if
sidecar_dest.exists() then if sidecar_dest.is_dir() call
shutil.rmtree(sidecar_dest), elif sidecar_dest.is_file() or
sidecar_dest.is_symlink() call sidecar_dest.unlink() (or os.remove), otherwise
handle other special file types appropriately, then proceed to
shutil.copytree(sidecar_src, sidecar_dest); apply this change where sidecar_src
and sidecar_dest are set.
- Around line 364-367: The code unconditionally removes the sidecar_dir in
remove_command(), affecting non-passthrough targets; change it to only remove
the co-named sidecar for passthrough/file-based command targets by gating the
shutil.rmtree call behind a check (e.g., if getattr(self, "is_passthrough",
False) or getattr(self, "file_based", False) or isinstance(self,
PassthroughCommandTarget)): compute sidecar_dir = dest_dir / Path(filename).stem
as before but only call sidecar_dir.is_dir() and shutil.rmtree(sidecar_dir) when
that passthrough/file-based predicate is true; update or add the minimal boolean
property/method on relevant subclasses instead of altering unrelated targets
that inherit BaseAssistantTarget.
---
Nitpick comments:
In `@tests/test_targets.py`:
- Around line 217-270: Add two tests mirroring the existing ClaudeCodeTarget
sidecar behavior: one that ensures the Cursor sidecar directory (create a
co-named directory next to the command source with nested files) is copied when
calling the Cursor target's generate_command (reference CursorTarget or the
Cursor-specific target class), and another that ensures
OpenCodeTarget.remove_command actually removes the sidecar directory when
uninstalling (assert that after calling OpenCodeTarget.remove_command the
co-named directory under the destination no longer exists). Implement these
tests analogous to test_generate_command_copies_sidecar_directory and
test_generate_command_overwrites_existing_sidecar so they create source
sidecars, call the appropriate target methods, then assert presence/absence of
files; reference the generate_command method and OpenCodeTarget.remove_command
to locate the relevant behavior to test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4da590d-29a3-41ca-8a74-2c8aec685be8
📒 Files selected for processing (3)
AGENTS.mdsrc/lola/targets/base.pytests/test_targets.py
7a6dda5 to
cd7cf1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lola/targets/base.py (1)
95-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis abstract API change is currently breaking typecheck.
CI is already failing with invalid overrides in
tests/test_targets_base.pyfor theseAssistantTargetmethods. The remaining subclasses/test doubles need to be updated to the new signatures before merge, otherwise this contract expansion keeps the PR red.Also applies to: 116-124, 139-147, 163-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lola/targets/base.py` around lines 95 - 103, The base API for generate_skill has expanded to include project_path: str | None = None and a keyword-only module_dir: Path | None = None, which breaks existing overrides; update every AssistantTarget subclass and test double that overrides generate_skill to add the new parameters with matching type annotations and defaults and forward or use them as appropriate, and make identical signature fixes to the other base methods referenced (the overrides corresponding to the diffs at 116-124, 139-147, 163-168) so their signatures match the base class and keep the return type bool.tests/test_targets_base.py (1)
45-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
MockTargetmethod signatures to matchAssistantTarget
tests/test_targets_base.pydefinesMockTarget.generate_skill(),generate_command(), andgenerate_agent()with kw-only parameter_module_dir, which doesn’t matchAssistantTarget’s kw-onlymodule_dir: Path | None(and the override stubs are missing matching type/return annotations). This is whyinvalid-method-overrideis triggered and whymodule_dir=...keyword calls would be rejected by this mock.Suggested fix pattern
- def generate_skill(self, _source_path, _dest_path, _skill_name, _project_path=None, *, module_dir=None): + def generate_skill( + self, + _source_path: Path, + _dest_path: Path, + _skill_name: str, + _project_path: str | None = None, + *, + module_dir: Path | None = None, + ) -> bool: return True - def generate_command(self, _source_path, _dest_dir, _cmd_name, _module_name, *, module_dir=None): + def generate_command( + self, + _source_path: Path, + _dest_dir: Path, + _cmd_name: str, + _module_name: str, + *, + module_dir: Path | None = None, + ) -> bool: return True - def generate_agent( - self, - _source_path, - _dest_dir, - _agent_name, - _module_name, - *, - _module_dir=None, - ): + def generate_agent( + self, + _source_path: Path, + _dest_dir: Path, + _agent_name: str, + _module_name: str, + *, + module_dir: Path | None = None, + ) -> bool: return TrueApply the same “signature + return type” alignment to the other stubbed overrides in this block (
remove_skill,remove_instructions,generate_skills_batch,get_command_filename,get_agent_filename,generate_mcps,remove_mcps,remove_command,remove_agent), and then rerunruff check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_targets_base.py` around lines 45 - 104, Update MockTarget method signatures to exactly match AssistantTarget: change kw-only parameter names to module_dir: Path | None (not _module_dir), add matching return type annotations (e.g., -> bool or -> Path | str where appropriate) and align parameter types for generate_skill, generate_command, generate_agent, generate_instructions, remove_skill, remove_instructions, generate_skills_batch, get_command_filename, get_agent_filename, generate_mcps, remove_mcps, remove_command, and remove_agent; ensure default/kw-only placement is preserved so calls like module_dir=... are accepted and then run ruff check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev-guide/architecture.md`:
- Around line 45-46: Update the architecture doc's numbered list (the "Skills"
and "Commands" items) to qualify that copying/registering of sidecar directories
applies only to passthrough/file-based command targets; explicitly note that
Gemini commands are generated as TOML and do not install markdown sidecar
directories so the description matches the implementation and PR objective.
In `@docs/guides/creating-modules.md`:
- Around line 88-99: The fenced code block showing the module directory tree is
missing a language tag; update the opening fence from ``` to a language label
(for example ```text) for the block that contains the my-module/module directory
tree so markdownlint stops flagging it.
- Around line 101-102: The current text promises a boundary-delimited module
directory preamble for every generated command, but Gemini emits TOML commands
so that statement is too broad; update the sentence that begins "Generated
skill, command, and agent files receive a boundary-delimited module directory
preamble..." to explicitly scope the preamble to generated skills, agents, and
file-based markdown command outputs (e.g., "Generated skill and agent files, and
file-based markdown commands, receive..."), and add a clarifying clause that
Gemini-emitted TOML command files do not include the markdown preamble.
In `@src/lola/targets/install.py`:
- Around line 227-230: The Cursor branch currently checks for a nonexistent
`.mdc` file so installed Cursor skills are never detected; update the check in
the function that uses target.name (used by _install_skills()) to detect Cursor
installs by testing the actual install directory or SKILL.md file that
CursorTarget.generate_skill() writes (e.g. (skill_dest / skill_name /
"SKILL.md").exists() or (skill_dest / skill_name).exists()), so the code will
correctly honor force=False and prompt/skip overwrites.
---
Outside diff comments:
In `@src/lola/targets/base.py`:
- Around line 95-103: The base API for generate_skill has expanded to include
project_path: str | None = None and a keyword-only module_dir: Path | None =
None, which breaks existing overrides; update every AssistantTarget subclass and
test double that overrides generate_skill to add the new parameters with
matching type annotations and defaults and forward or use them as appropriate,
and make identical signature fixes to the other base methods referenced (the
overrides corresponding to the diffs at 116-124, 139-147, 163-168) so their
signatures match the base class and keep the return type bool.
In `@tests/test_targets_base.py`:
- Around line 45-104: Update MockTarget method signatures to exactly match
AssistantTarget: change kw-only parameter names to module_dir: Path | None (not
_module_dir), add matching return type annotations (e.g., -> bool or -> Path |
str where appropriate) and align parameter types for generate_skill,
generate_command, generate_agent, generate_instructions, remove_skill,
remove_instructions, generate_skills_batch, get_command_filename,
get_agent_filename, generate_mcps, remove_mcps, remove_command, and
remove_agent; ensure default/kw-only placement is preserved so calls like
module_dir=... are accepted and then run ruff check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bd3331b-5503-486d-99fe-c0e352579fbe
📒 Files selected for processing (18)
.gitignoreAGENTS.mdREADME.mddocs/dev-guide/architecture.mddocs/guides/creating-modules.mddocs/guides/modules.mdsrc/lola/targets/base.pysrc/lola/targets/claude_code.pysrc/lola/targets/cursor.pysrc/lola/targets/gemini.pysrc/lola/targets/install.pysrc/lola/targets/openclaw.pysrc/lola/targets/opencode.pytests/test_cli_install.pytests/test_installer.pytests/test_instructions.pytests/test_targets.pytests/test_targets_base.py
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- docs/guides/modules.md
- README.md
- AGENTS.md
16c1d87 to
ed9626b
Compare
|
Had to refactor this a couple of times based on experimentation. It turns out that LLMs will ignore blocks of text in the |
ed9626b to
0bfb743
Compare
|
Rebased |
0bfb743 to
e4f9cca
Compare
|
Rebased |
mrbrandao
left a comment
There was a problem hiding this comment.
This looks good to me, thanks for adding this.
I small note, would be cool to mention also
|
This code is good to merge; the only issue is more related to the markdown formatter, which would be better to have in a separate PR, to avoid the noise. |
Install now copies the full module content tree to each target's modules/<name>/ directory, keeping internal relative paths intact (convention packs, reference docs, etc.). Commands can now have co-named sidecar directories. A command like commands/review.md can ship with commands/review/ holding supporting files that get copied alongside it. Every installed asset (skill, command, agent) gets a plain-text module-dir preamble block between frontmatter and body: Module root: <path> This is the installed root of the module. Resolve all relative file references in this document against the path above. Example: packs/foo.md -> <path>/packs/foo.md Gemini CLI commands use TOML, so they skip the preamble. The ManagedSectionTarget writes a Markdown-formatted module directory line in the skills batch section instead. Also adds `ty check` to `make lint` to match CI. Fixes: LobsterTrap#171
e4f9cca to
e795ec1
Compare
Multi-file commands pair an entry file (commands/.md) with a co-named directory (commands//) of supporting files. Only the entry file was copied to the target, so relative-path references from the entry into the sidecar directory dangled at runtime.
_generate_passthrough_command now copies the sidecar directory next to the entry file, replacing any stale copy on reinstall. remove_command cleans up both the entry and the sidecar directory on uninstall.
Fixes: #171
Checklist
pytest)ruff check src tests)ty check)AI Disclosure
Summary by CodeRabbit
New Features
Documentation
Tests
Chores